url: update the processing in the scheme state. #11887

watilde · 2017-03-17T01:20:43Z

Fix #11785. I've updated the processing in the scheme state to follow the specification and synchronized url-setter-tests with upstream.

Related lines in the spec:
https://github.com/whatwg/url/blob/a8ed8d2aefeec1e742490d08844c3d91a87e77fb/url.bs#L1493-L1508

While I was testing added tests, I noticed that the current state machine of Node.js doesn't parse URL correctly for pathname. For example:

var url = new URL('ssh://example.net')
url.pathname
// expected: //example.net
// actual: '/'

So I got rid of the following test for this time.

{
  "href": "ssh://example.net",
  "new_value": "file",
  "expected": {
    "href": "ssh://example.net",
    "protocol": "ssh:"
  }
},

I will work on it later.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests are included
commit message follows [commit guidelines][]

Affected core subsystem(s)

url, test

watilde · 2017-03-17T01:29:48Z

cc @nodejs/url

TimothyGu · 2017-03-17T02:23:08Z

The returning is handled in the JS layer. You might want to update that instead.

watilde · 2017-03-17T02:35:39Z

Oh that's nice. I will update the commit then!

watilde · 2017-03-17T03:01:57Z

@TimothyGu I've updated the commits to handling it in JS layer.

Well, I'm not sure those steps should be handled in the scheme state or not, but at least the spec expects it as scheme state checking. What are your thoughts on?

TimothyGu · 2017-03-17T15:19:33Z

lib/internal/url.js

@@ -133,6 +133,12 @@ function onParseProtocolComplete(flags, protocol, username, password,
  if ((s && !newIsSpecial) || (!s && newIsSpecial)) {
    return;
  }
+  if (protocol === 'file:' && (username || password || port > -1)) {


port can only be >= 0 or undefined, since the C++ layer sets port to undefined if it is -1, and you should probably check ctx.username and ctx.password instead of username and password since IIRC the C++ layer doesn't pass those into the callback if it's unneeded.

Good catch. +1

That's right! I will update it. Thanks 😺

Ok, I've updated.

watilde · 2017-03-17T20:12:56Z

ci: https://ci.nodejs.org/job/node-test-pull-request/6908/

TimothyGu

LGTM other than the nit.

TimothyGu · 2017-03-17T22:15:24Z

lib/internal/url.js

@@ -133,6 +133,14 @@ function onParseProtocolComplete(flags, protocol, username, password,
  if ((s && !newIsSpecial) || (!s && newIsSpecial)) {
    return;
  }
+  if (protocol === 'file:' &&
+    (ctx.username || ctx.password || ctx.port !== undefined)
+  ) {


nit: stylistically it's more common to write it as:

if (protocol === 'file:' && (ctx.username || ctx.password || ctx.port !== undefined)) {

Since file URLs can not have `username/password/port`, the specification was updated to restrict setting protocol to "file". Refs: whatwg/url#269 Fixes: nodejs#11785

Synchronize url-setter-test to upstream. Refs: web-platform-tests/wpt#5112

watilde · 2017-03-18T08:01:42Z

new CI() https://ci.nodejs.org/job/node-test-pull-request/6912/

joyeecheung

If there are test cases that don't pass at the moment, you can change url-setter-tests.json to a js file with module.exports, then put those test cases in that file and comment them out, like what https://github.com/nodejs/node/blob/master/test/fixtures/url-tests.js does

watilde · 2017-03-20T14:28:32Z

@joyeecheung that's nice! I will sync the setter url fixture in my next patch, will rename the file at the same time then :)

watilde · 2017-03-20T18:46:33Z

Landed in 60c8a35 and cab58fe.

Since file URLs can not have `username/password/port`, the specification was updated to restrict setting protocol to "file". Refs: whatwg/url#269 Fixes: #11785 PR-URL: #11887 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Synchronize url-setter-test to upstream. Refs: web-platform-tests/wpt#5112 PR-URL: #11887 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Since file URLs can not have `username/password/port`, the specification was updated to restrict setting protocol to "file". Refs: whatwg/url#269 Fixes: nodejs#11785 PR-URL: nodejs#11887 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Synchronize url-setter-test to upstream. Refs: web-platform-tests/wpt#5112 PR-URL: nodejs#11887 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Since file URLs can not have `username/password/port`, the specification was updated to restrict setting protocol to "file". Refs: whatwg/url#269 Fixes: #11785 PR-URL: #11887 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Synchronize url-setter-test to upstream. Refs: web-platform-tests/wpt#5112 PR-URL: #11887 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Refs: web-platform-tests/wpt#4586 Refs: nodejs#11887

Updates: + Bring tests url-setter-tests from WPT, and put it as JavaScript + Comment out unpassed tests Refs: web-platform-tests/wpt#5112 Refs: nodejs#11887

Refs: web-platform-tests/wpt#4586 Refs: #11887 PR-URL: #12058 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

Updates: + Bring tests url-setter-tests from WPT, and put it as JavaScript + Comment out unpassed tests Refs: web-platform-tests/wpt#5112 Refs: #11887 PR-URL: #12058 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

Updates: + Bring tests url-setter-tests from WPT, and put it as JavaScript + Comment out unpassed tests Refs: web-platform-tests/wpt#5112 Refs: nodejs#11887 PR-URL: nodejs#12058 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

PR-URL: nodejs#12507 Refs: web-platform-tests/wpt#4586 Refs: nodejs#11887 Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #12507 Refs: web-platform-tests/wpt#4586 Refs: #11887 Reviewed-By: James M Snell <jasnell@gmail.com>

watilde added c++ Issues and PRs that require attention from people who are familiar with C++. test Issues and PRs related to the tests. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Mar 17, 2017

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Mar 17, 2017

watilde force-pushed the feature/file-url branch from af99bab to 1edd461 Compare March 17, 2017 02:56

watilde removed the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 17, 2017

jasnell approved these changes Mar 17, 2017

View reviewed changes

TimothyGu suggested changes Mar 17, 2017

View reviewed changes

watilde force-pushed the feature/file-url branch from 1edd461 to 6ccf827 Compare March 17, 2017 19:37

TimothyGu approved these changes Mar 17, 2017

View reviewed changes

watilde added 2 commits March 18, 2017 08:35

url: restrict setting protocol to "file"

9d4d73f

Since file URLs can not have `username/password/port`, the specification was updated to restrict setting protocol to "file". Refs: whatwg/url#269 Fixes: nodejs#11785

test: synchronize WPT url setters tests data

18eebe9

Synchronize url-setter-test to upstream. Refs: web-platform-tests/wpt#5112

watilde force-pushed the feature/file-url branch from 6ccf827 to 18eebe9 Compare March 18, 2017 07:36

TimothyGu approved these changes Mar 18, 2017

View reviewed changes

joyeecheung approved these changes Mar 20, 2017

View reviewed changes

watilde closed this Mar 20, 2017

watilde deleted the feature/file-url branch March 20, 2017 18:46

watilde mentioned this pull request Mar 27, 2017

url: change path parsing for non-special URLs #12058

Closed

3 tasks

MylesBorins mentioned this pull request Mar 28, 2017

V7.8.0 proposal #12104

Merged

watilde added a commit to watilde/node that referenced this pull request Apr 2, 2017

test: synchronize WPT url test data

616a59a

Refs: web-platform-tests/wpt#4586 Refs: nodejs#11887

watilde added a commit to watilde/node that referenced this pull request Apr 2, 2017

test: synchronize WPT url setter test data

b103e6e

Updates: + Bring tests url-setter-tests from WPT, and put it as JavaScript + Comment out unpassed tests Refs: web-platform-tests/wpt#5112 Refs: nodejs#11887

italoacasas mentioned this pull request Apr 10, 2017

v7.9.0 Release Proposal #12319

Merged

2 tasks

TimothyGu pushed a commit to TimothyGu/node that referenced this pull request Apr 25, 2017

test: synchronize WPT url test data

dceb12e

PR-URL: nodejs#12507 Refs: web-platform-tests/wpt#4586 Refs: nodejs#11887 Reviewed-By: James M Snell <jasnell@gmail.com>

evanlucas pushed a commit that referenced this pull request May 1, 2017

test: synchronize WPT url test data

1adb08e

PR-URL: #12507 Refs: web-platform-tests/wpt#4586 Refs: #11887 Reviewed-By: James M Snell <jasnell@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

url: update the processing in the scheme state. #11887

url: update the processing in the scheme state. #11887

watilde commented Mar 17, 2017

watilde commented Mar 17, 2017

TimothyGu commented Mar 17, 2017

watilde commented Mar 17, 2017

watilde commented Mar 17, 2017

TimothyGu Mar 17, 2017

jasnell Mar 17, 2017

watilde Mar 17, 2017

watilde Mar 17, 2017

watilde commented Mar 17, 2017

TimothyGu left a comment

TimothyGu Mar 17, 2017

watilde commented Mar 18, 2017

joyeecheung left a comment

watilde commented Mar 20, 2017

watilde commented Mar 20, 2017

url: update the processing in the scheme state. #11887

url: update the processing in the scheme state. #11887

Conversation

watilde commented Mar 17, 2017

Checklist

Affected core subsystem(s)

watilde commented Mar 17, 2017

TimothyGu commented Mar 17, 2017

watilde commented Mar 17, 2017

watilde commented Mar 17, 2017

TimothyGu Mar 17, 2017

Choose a reason for hiding this comment

jasnell Mar 17, 2017

Choose a reason for hiding this comment

watilde Mar 17, 2017

Choose a reason for hiding this comment

watilde Mar 17, 2017

Choose a reason for hiding this comment

watilde commented Mar 17, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu Mar 17, 2017

Choose a reason for hiding this comment

watilde commented Mar 18, 2017

joyeecheung left a comment

Choose a reason for hiding this comment

watilde commented Mar 20, 2017

watilde commented Mar 20, 2017